Share one node_modules cache key so CI can actually restore it by jakebromberg · Pull Request #2257 · WXYC/Backend-Service

jakebromberg · 2026-08-24T01:52:37Z

Closes #2256.

The bug

GitHub Actions scopes every cache entry to the ref that wrote it. A pull_request run can read its own refs/pull/N/merge scope and the default branch's — nothing else. So the default branch is the only scope a cache can be shared through.

test.yml triggers only on pull_request and keyed its cache node-modules-*. deploy-base.yml is the one workflow that runs on push-to-main, and it keyed the same directory, built from the same lockfile hash, as deploy-node-modules-*.

One prefix apart. So nothing ever restored: every PR missed, ran npm ci, and saved another ~145 MB copy into a scope only that PR could read.

Measured before the change


Store	194 entries / 10.59 GB against GitHub's 10 GB per-repo cap — permanently evicting
`node-modules-*`	50 entries, 49 of them PR-scoped (6.93 GB)
Worst duplication	one lockfile hash with ten byte-identical copies, one per PR
`npm ci`	ran on 13 of 13 sampled branches, 25–34s each

The current lockfile hashes to ef5593458e…, and deploy-node-modules-…ef5593458e… was sitting in the main scope the whole time — the exact bytes every PR needed, under the wrong name.

Why unifying the prefix is the whole fix

actions/cache only saves on a miss. Once PRs restore from main's entry they stop writing per-PR copies, so the family collapses to roughly one entry per lockfile revision instead of one per PR. That addresses both acceptance criteria in #2256 — the restore and the size — with one change rather than a pruning cron.

It is self-healing after one merge: this PR's own merge runs deploy-auto → deploy-base, which writes node-modules-<current hash> into the main scope for every subsequent PR to read.

Second change: stop paying for a second cache that can never help

setup-node's package-manager-cache defaults to true whenever package.json declares packageManager — ours says npm@11.11.0. So every setup-node in this repo was caching ~169 MB of ~/.npm that nobody configured, including auth-tables-doc-drift, which installs nothing at all.

Alongside a node_modules tarball it can never pay for itself, because both key on hashFiles('package-lock.json'):

node_modules restores → npm never runs → ~/.npm goes unread
lockfile changes → ~/.npm misses too → it cannot speed up the reinstall

It is disabled in the six jobs that cache node_modules (or install nothing). migrate-dryrun and schema-shape-report.yml install a subset of deps without a node_modules tarball, genuinely benefit, and keep their explicit cache: 'npm' opt-in.

The guard

tests/unit/scripts/ci-node-modules-cache.test.ts — 14 tests pinning both invariants, plus a parser-sanity test so it can't pass vacuously. Verified it bites in both directions:

Reverted	Result
key prefix → `deploy-node-modules-*`	2 failures
drop one `package-manager-cache: false`	1 failure, naming the job

It strips YAML comment lines before matching. Its first cut tripped on its own explanatory prose, which quotes both cache: 'npm' and the retired prefix verbatim — a guard that reads documentation instead of configuration is exactly the vacuous-pass failure mode #2249 was about.

This is also the kind of spec that could not have been trusted before #2255: it reads workflow files as text, so Jest's affected-tests mode would never have selected it when a workflow changed.

Note

deploy-base.yml already routes buildx layers to ECR specifically to avoid thrashing this same 10 GB cap ("Don't switch to type=gha: … ~38 images' layer caches would thrash/evict each other"). The node_modules caches had quietly recreated the thrash it was avoiding.

Local verification

npm run typecheck — clean
npm run test:unit — 473 suites / 8196 tests, all passing
eslint — 0 errors (5 warnings on the new spec, all security/detect-non-literal-*, same class its siblings emit)
npm run check:docs — 7 findings, all pre-existing in docs/migrations.md
npm run check:auth-tables-doc — PASS
prettier --check — clean

GitHub Actions scopes every cache entry to the ref that wrote it. A `pull_request` run can only read its own `refs/pull/N/merge` scope plus the default branch's, so the default branch is the only scope a cache can be shared through. `test.yml` triggers only on `pull_request` and keyed its cache `node-modules-*`; `deploy-base.yml` is the sole workflow running on push-to-main, and keyed the same directory, built from the same lockfile hash, as `deploy-node-modules-*`. One prefix apart, so nothing ever restored: every PR missed, ran `npm ci`, and saved another ~145 MB copy into a scope only it could read. Measured before this change: 194 entries / 10.59 GB against GitHub's 10 GB per-repo cap, so the store was permanently evicting. 49 of the 50 `node-modules-*` entries were PR-scoped (6.93 GB), and one lockfile hash had ten byte-identical copies, one per PR. `npm ci` ran on 13 of 13 sampled branches at 25-34s each. The current lockfile hashes to ef5593458e..., and `deploy-node-modules-...ef5593458e...` was sitting in the main scope the whole time — the exact bytes every PR needed, under the wrong name. Unifying the prefix fixes the write side too. `actions/cache` only saves on a miss, so once PRs restore from main they stop writing per-PR copies, and the family collapses to roughly one entry per lockfile revision. Also turn off setup-node's automatic `~/.npm` cache wherever a job already caches `node_modules`. It is on by default because package.json declares `packageManager` (npm@11.11.0), so it was running in jobs nobody configured for it, including `auth-tables-doc-drift`, which installs nothing at all. It cannot pay for itself alongside a `node_modules` tarball: both key on hashFiles('package-lock.json'), so they hit and miss together — when node_modules restores, npm never runs; when the lockfile changes, ~/.npm misses too. That is ~169 MB per lockfile revision per job. `migrate-dryrun` and `schema-shape-report.yml` install a subset of deps without a node_modules tarball, genuinely benefit, and keep their explicit `cache: 'npm'` opt-in. `tests/unit/scripts/ci-node-modules-cache.test.ts` pins both invariants. Reverting either half turns it red: the key prefix fails 2 assertions, dropping a single `package-manager-cache: false` fails 1 and names the job. The guard reads YAML with comment lines stripped — its first cut tripped on its own explanatory prose, which quotes both `cache: 'npm'` and the retired prefix verbatim. deploy-base.yml already routes buildx layers to ECR specifically to avoid thrashing this same 10 GB cap. The node_modules caches had quietly recreated the thrash it was avoiding. Closes #2256

jakebromberg · 2026-08-24T02:02:23Z

CI green — run 32681188331, all jobs passing (Migration Dry-Run skipped, which satisfies protection).

The cache log confirms the mechanism, including one thing I didn't plan for:

unit-tests          Cache not found for input keys: node-modules-Linux-node24-ef5593458e…
unit-tests          Post: Cache saved with key:    node-modules-Linux-node24-ef5593458e…
lint-and-typecheck  Cache not found for input keys: node-modules-Linux-node24-ef5593458e…
Integration-Tests   Cache restored from key:        node-modules-Linux-node24-ef5593458e…

unit-tests and lint-and-typecheck both miss — expected, and stated up front: the main scope won't hold a node-modules-* entry at this hash until this PR merges and deploy-auto runs. They started within two seconds of each other, so neither could see the other's save.

Integration-Tests starts ~5 minutes later and restores — from the entry unit-tests had just written into this PR's own scope. That is the whole bug in one run: sharing a cache works fine, so long as the reader is inside the scope of the writer. Cross-PR, the only shared scope is the default branch's, and the prefix mismatch meant nothing was ever there to read.

The npm-cache half is confirmed by absence: zero node-cache-* writes in the entire run. All four Post Set Up Node.js steps report a bare "Post job cleanup." Before this change each of those jobs would have saved its own ~169 MB ~/.npm tarball — about 676 MB of writes avoided in this single run, none of which could ever have been read by a job that also restores node_modules.

Job durations, for the record: unit-tests 121s (holding at the 122s measured after #2255, so the full-suite change hasn't drifted), lint-and-typecheck 306s, Integration-Tests 158s. lint-and-typecheck is above its usual ~238s because this is a fresh branch missing both the node_modules and the tool-cache entries.

AC#1 is not verifiable on this PR by construction — it needs a merge first. I'll confirm it on the next PR that lands afterward.

…ards Review found the guard could be skipped on exactly the PRs it exists to catch. `unit-tests` is gated on `detect-changes.outputs.src`, whose paths filter listed `.github/workflows/test.yml` but not `deploy-base.yml`. A PR reverting deploy-base's cache key — the literal defect — touches no `src` path, so `unit-tests` never runs, the required check reports `skipped`, and branch protection accepts that as passing. This is the second time that trap has sprung: BS#1807 added `test.yml` to `src` for the same reason, one file at a time. Widened to `.github/workflows/**`, since the guard globs the whole directory. Drop the `package-manager-cache: false` half entirely. Its stated rationale — npm never runs when node_modules restores, so a ~/.npm cache can never pay for itself — is false in two of the six jobs it touched. Integration-Tests runs `npm ci` in dev_env/mock-api-server (express + typescript + @types), a separate project whose deps root node_modules never contains, and deploy-base runs `npm ci` unconditionally. Worse, deploy-base and nightly-tests are the only runs that write `node-cache-*` on the default branch, so disabling it there would have starved migrate-dryrun and schema-shape-report.yml — both pull_request-only, both keeping an explicit `cache: 'npm'` — of the only scope they can restore from. That is the same unrestorable-scope bug this PR is fixing, inverted. It is a separable optimization that needs measurement, not a rider on this change. Correct the sole-writer claim. `nightly-tests.yml` is `schedule`-triggered, and scheduled runs execute on the default branch, so it also writes the shared key there. It was never true that nothing could populate the shared scope. The real asymmetry is cadence: nightly fires once a day and its entry goes stale the moment the lockfile moves, which is precisely the state main was found in — one `node-modules-*` entry, at a stale hash. deploy-base runs on every merge, which is what keeps a current entry in the shared scope. Close a silent-decay hole in the guard. `nodeModulesCacheSteps` required `path` to be an inline scalar, so rewriting one step as a block list — a natural edit when adding a second directory — would have dropped it out of the checked set while every assertion kept passing. It now parses both forms and pins the step count exactly rather than as a floor. Verified by mutation: reverting the key prefix fails 2 assertions, narrowing the src filter fails 1, and rewriting a path as a block list keeps the step in the checked set rather than silently exempting it.

jakebromberg · 2026-08-24T02:22:28Z

Review addressed in 5bde67df. Four of the six findings were valid; the two HIGHs both landed, and one of them was the most useful thing anyone said about this PR.

HIGH — src filter skips the guard. Correct, and worse than stated: this is the second occurrence. BS#1807 added .github/workflows/test.yml to src for exactly this reason — a guard that reads a file must be triggered by changes to that file — and enumerating them one at a time left deploy-base.yml out. A PR reverting the cache key would have skipped unit-tests, and a skipped required check satisfies branch protection. Widened to .github/workflows/**, since the guard globs the whole directory, with a comment saying to keep it a directory glob. Now pinned by a test: narrowing it back to a single file fails.

HIGH — disabling package-manager-cache starves the PR-only consumers. Correct, and it exposed that my rationale was wrong on its own terms. I've dropped that half of the PR entirely rather than patch it.

Two things killed it:

Your finding Auth middleware #3 is the cleaner disproof. Integration-Tests runs cd dev_env/mock-api-server && npm ci unconditionally (test.yml:456) — a separate project with its own lockfile (express, typescript, @types/node) whose deps root node_modules never contains. So "when node_modules restores, npm never runs" is simply false there, independent of any scope argument.
Your finding Fixed error handling on /flowsheet/end and changed http method to POST #2 is the same bug this PR fixes, inverted: deploy-base.yml and nightly-tests.yml are the only default-branch writers of node-cache-*, and migrate-dryrun / schema-shape-report.yml are pull_request-only. Removing the writers would leave the consumers permanently unable to restore.

One correction to the finding, which doesn't change the conclusion: the two keys are the same hash. setup-node keys on root-only package-lock.json, not **/package-lock.json — verified by reproducing hashFiles (for a single file it's sha256 of the file's raw digest), which yields ef5593458e…, matching both node-cache-Linux-x64-npm-ef5593458e… and deploy-node-modules-Linux-node24-ef5593458e…. And the PR-scoped node-cache-* entries are ~700 bytes each (20,158 bytes across 29), not 0 — they missed and saved a near-empty cache, which weakens the "they're restoring 169 MB today" premise. Neither rescues my version, so it's out. Filed as #2259 with the measurements.

LOW — the sole-writer claim is wrong. Correct. nightly-tests.yml is schedule-triggered and scheduled runs execute on the default branch, so it also writes the shared key there. Fixed in the comment, the docs rule, and the spec header. The corrected version is a better argument anyway: it was never true that nothing could fill the shared scope — the asymmetry is cadence. Nightly fires once a day and its entry goes stale the moment the lockfile moves, which is exactly the state main was found in (one node-modules-* entry, at a stale hash). deploy-base runs on every merge, which is what keeps a current entry there.

LOW — the guard can decay silently. Correct. path is now parsed in both scalar and block form, and the step count is pinned exactly (5) rather than as a floor, with a comment explaining that a floor cannot catch a step dropping out of the checked set. Verified: rewriting path: node_modules as a block list with a second directory keeps the step in the set instead of exempting it.

LOW — deploy-base restores a tarball npm ci immediately wipes. Agreed and pre-existing; I'm leaving it. The save is what populates the shared scope, so the step has to stay; only the restore is wasted. Guarding the install the way the other four sites do is a change to the deploy path and belongs in its own PR — noted in #2259's neighbourhood rather than smuggled in here.

Net diff is now just the key prefix, the trigger fix, the guard, and the doc rule. nightly-tests.yml is untouched.

Re-verified locally: typecheck clean, 473 suites / 8188 tests passing, eslint 0 errors, prettier clean, check:auth-tables-doc PASS.

CI verification of the previous commit turned up a second defect that made the first one moot: every job that restored the cache printed "Cache corrupted, will reinstall", deleted node_modules, and ran `npm ci` anyway. The cache had never saved a second even when it hit. This is an npm workspaces repo (`apps/*`, `jobs/*`, `shared/*`) and dependencies do not all hoist — `apps/backend`, `apps/auth` and `shared/authentication` each carry their own node_modules. Every cache step cached `path: node_modules`, the root only. On restore the workspaces' own deps are missing, so `npm ls --depth=0` reports root's hoisted copies as `invalid` against the workspace ranges and exits 1, which is exactly what the `Validate cached node_modules` gate treats as corruption. Reproduced locally: moving `apps/backend/node_modules` aside flips `npm ls --depth=0` from 0 to 1 with `axios@1.18.1 invalid: "^1.19.0" from apps/backend`. So AC#1 of #2256 — a PR that doesn't touch package-lock.json skips Install Dependencies — was unreachable by the key fix alone. A partial tree under a shared key is worse than no cache: it pays the download and then reinstalls. The path list now mirrors package.json's `workspaces`. The key gains a `-v2` because the path set changed while the lockfile hash did not, and actions/cache never overwrites an existing key — without the bump every job would keep restoring the old, incomplete entry. The guard pins the path list as well as the key, since a shared key over differing path sets is its own correctness bug. Verified by mutation: reverting one step to `path: node_modules` fails the new assertion. Docs rule compressed to a pointer — check-doc-rules flagged it as a compress-candidate once the test carried the load, which is the convention that script exists to enforce.

jakebromberg · 2026-08-24T02:38:28Z

AC#1 is now demonstrated on this PR — and getting there turned up a second defect that made the first fix moot on its own.

Verifying the previous commit, I noticed every job that restored the cache printed ⚠️ Cache corrupted, will reinstall, deleted node_modules, and ran npm ci anyway. The cache had never saved a second even when it hit — the scope bug had been hiding a second one behind it.

Cause: this is an npm workspaces repo (apps/*, jobs/*, shared/*) and deps don't all hoist — apps/backend, apps/auth and shared/authentication each carry their own node_modules. Every cache step cached path: node_modules, the root only. On restore the workspaces' own deps are missing, so npm ls --depth=0 reports root's hoisted copies as invalid against the workspace ranges and exits 1 — exactly what Validate cached node_modules treats as corruption. Reproduced locally:

$ mv apps/backend/node_modules /tmp/ && npm ls --depth=0; echo $?
│ ├── axios@1.18.1 invalid: "^1.19.0" from apps/backend
│ ├── better-auth@1.6.26 invalid: "^1.6.30" from apps/backend
1
$ mv /tmp/node_modules apps/backend/ && npm ls --depth=0 >/dev/null; echo $?
0

The path list now mirrors package.json's workspaces. The key gains -v2 because the path set changed while the lockfile hash did not, and actions/cache never overwrites an existing key — without the bump every job would keep restoring the old, incomplete entry.

The two runs side by side, same PR, same lockfile:

	before (`node-modules-*`)	after (`node-modules-v2-*`)
`Integration-Tests` cache	restored	restored
`Validate cached node_modules`	"Cache corrupted, will reinstall"	success
`Install Dependencies`	ran	skipped ✅
corruption events in run	3 (all restoring jobs)	0

unit-tests and lint-and-typecheck still miss on this run — expected, -v2 is a brand-new key and they start in parallel before anything has saved it. They now save the complete tree, which is what Integration-Tests then restores.

That is AC#1's exact mechanism, just proven within one PR's scope instead of across two. Cross-PR it needs the merge first, so deploy-auto writes node-modules-v2-… into the main scope.

The guard now pins the path list alongside the key — a shared key over differing path sets is its own correctness bug, since a partial tree under a key that promises a full one is worse than no cache at all. Reverting one step to path: node_modules fails the new assertion.

Re-verified: typecheck clean, 473 suites / 8189 tests, eslint 0 errors, prettier clean, and check-doc-rules now reports zero findings for this rule (it flagged the prose as a compress-candidate once the test carried the load, so I compressed it to a pointer — which is the convention that script exists to enforce).

jakebromberg mentioned this pull request Aug 24, 2026

GitHub Actions node_modules cache never restores: the store sits at the 10 GB cap and evicts entries before they can be reused #2256

Closed

jakebromberg mentioned this pull request Aug 24, 2026

setup-node caches ~/.npm in every job by default; decide per-job whether it earns its 169 MB #2259

Open

jakebromberg merged commit 207697d into main Aug 24, 2026
6 checks passed

jakebromberg deleted the fix/2256-unify-node-modules-cache-key branch August 24, 2026 03:01

Navigation Menu

Uh oh!

Share one node_modules cache key so CI can actually restore it - #2257

Share one node_modules cache key so CI can actually restore it#2257
jakebromberg merged 3 commits into
mainfrom
fix/2256-unify-node-modules-cache-key

jakebromberg commented Aug 24, 2026

Uh oh!

jakebromberg commented Aug 24, 2026

Uh oh!

jakebromberg commented Aug 24, 2026

Uh oh!

jakebromberg commented Aug 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jakebromberg commented Aug 24, 2026

The bug

Measured before the change

Why unifying the prefix is the whole fix

Second change: stop paying for a second cache that can never help

The guard

Note

Local verification

Uh oh!

jakebromberg commented Aug 24, 2026

Uh oh!

jakebromberg commented Aug 24, 2026

Uh oh!

jakebromberg commented Aug 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant